-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[bazel][libc] Remove target compatibility restrictions for float128 #169292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-libc Author: Chandler Carruth (chandlerc) ChangesThe restrictions here aren't nearly as much about the OS as the compiler and architecture, but the Bazel restriction was OS-based. Everything seems to work well on even Arm64 macOS, and I would expect most BSDs and other OSes to work well with Clang's support on x86-64. The source code here already handles detecting when there is compiler support for the type. And the users of this don't Full diff: https://github.com/llvm/llvm-project/pull/169292.diff 1 Files Affected:
diff --git a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
index bd48222856f22..e3962d9b5f95b 100644
--- a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
@@ -171,11 +171,6 @@ libc_support_library(
libc_support_library(
name = "llvm_libc_types_float128",
hdrs = ["include/llvm-libc-types/float128.h"],
- target_compatible_with = select({
- "@platforms//os:linux": [],
- "@platforms//os:windows": [],
- "//conditions:default": ["@platforms//:incompatible"],
- }),
deps = [":llvm_libc_macros_float_macros"],
)
|
The restrictions here aren't nearly as much about the OS as the compiler and architecture, but the Bazel restriction was OS-based. Everything seems to work well on even Arm64 macOS, and I would expect most BSDs and other OSes to work well with Clang's support on x86-64. The source code here already handles detecting when there is compiler support for the type. And the users of this don't `select` or do anything else to conditionally include the header, so it seems better to not restrict access to the header from the build system, and instead continue making the source code compatible or a no-op on relevant configurations.
94325ba to
5b3f738
Compare
|
(rebasing -- seemed to pick a bad baseline that didn't build MLIR cleanly, but unrelated to this PR) |
|
A bunch of targets were tagged w/ this in #86174, but it looks like this one was tagged by mistake, or maybe the sources didn't use to properly guard linux-specific bits. |
|
Thanks for review! Bazel seems still busted for MLIR, unrelated to this PR, so merging. |
…lvm#169292) The restrictions here aren't nearly as much about the OS as the compiler and architecture, but the Bazel restriction was OS-based. Everything seems to work well on even Arm64 macOS, and I would expect most BSDs and other OSes to work well with Clang's support on x86-64. The source code here already handles detecting when there is compiler support for the type. And the users of this don't `select` or do anything else to conditionally include the header, so it seems better to not restrict access to the header from the build system, and instead continue making the source code compatible or a no-op on relevant configurations.
This builds on the previous work to flesh out more on-demand runtimes building. It adds building of the `libc++.a` archive runtime. A number of changes are required for this to work: - The runtimes build infrastructure needs to support building sources from multiple parts of LLVM rather than a single part. We do this by lifting the root of the runtimes source paths up a level to a common runtimes tree, and installing the runtimes sources below this directory. - Both libc++ and libc++abi runtimes sources need to be installed, and we even need to install some interesting parts of llvm-libc that are used in the build of libc++. - We need to generate the site configuration header file for libc++ from the CMake template. This includes both setting up a set of platform-independent defines and introducing some basic Bazel support for processing the CMake template itself. Doing all of this also exposed some missing features and limitations of the runtimes building infrastructure that are addressed here. One note is that all of this just adds libc++ to the explicit `build-runtimes` command for testing. It doesn't yet trigger automatically building these prior to linking, or configuring any of the other subcommands to automatically use these runtimes. All of that will come in follow-up PRs. Also, this makes the `clang_runtimes_test` ... _very_ slow in our default build configuration. Compiling libc++, even with many threads on a large Linux server requires up to 50 seconds. I'm open to any suggestions on how to handle this, including disabling the test in non-optimized builds. I have some ideas to speed this up, but fundamentally building libc++ is... not cheap. I did look at some of the existing Bazel tools to process the CMake template, but they all seemed significantly more complex than what we need and didn't have broad adoption. Given that, it seemed slightly better to just roll our own given the simple format. Two of the new LLVM patch are currently under review upstream and so hopefully temporary: - llvm/llvm-project#169155 - llvm/llvm-project#169292
The restrictions here aren't nearly as much about the OS as the compiler and architecture, but the Bazel restriction was OS-based. Everything seems to work well on even Arm64 macOS, and I would expect most BSDs and other OSes to work well with Clang's support on x86-64.
The source code here already handles detecting when there is compiler support for the type. And the users of this don't
selector do anything else to conditionally include the header, so it seems better to not restrict access to the header from the build system, and instead continue making the source code compatible or a no-op on relevant configurations.